Some changes occurred in match checking

cc @Nadrieril

Some changes occurred in compiler/rustc_passes/src/check_attr.rs

cc @jdonszelmann

Some changes occurred in rustc_ty_utils::consts.rs

cc @BoxyUwU

Some changes occurred in exhaustiveness checking

cc @Nadrieril

Some changes occurred in match lowering

cc @Nadrieril

☔ The latest upstream changes (presumably #138974) made this pull request unmergeable. Please resolve the merge conflicts.

Only speaking of the MIR lowering part: my opinion on the current implementation is that this is a fine approach for an experiment, but this will need to change in depth before it can be relied on.

For one, I believe the static pattern-matching should use the const-eval interpreter instead of manually operating on valtrees. For two, it should not duplicate the work of match lowering; instead, BuiltMatchTree should track the tests cases that lead to each branch so that we can reuse them. This is the only way or-patterns can be supported properly.

Haven't reviewed the rest, would appreciate any help there, otherwise I'll get to it once approved.

In terms of the experiment, my current take is that using patterns for this doesn't pull its weight. I expect we won't allow guaranteed-direct-jump using a non-fully-const value like:

#[loop_match]
loop {
    state = 'blk: {
        match state {
            None => {
                let r = random();
                const continue 'blk Some(r);
            },
            Some(_) => break,
        }
    };
}

The reason being that this requires inspecting the expression which is a weird sort of abstraction break (you wouldn't be able to do let x = Some(r); const continue 'blk x;).

And without that, using patterns seems barely better than jump labels.

Thanks for having a look at the MIR lowering. That was indeed what I most wanted your eyes on.

For one, I believe the static pattern-matching should use the const-eval interpreter instead of manually operating on valtrees. For two, it should not duplicate the work of match lowering; instead, BuiltMatchTree should track the tests cases that lead to each branch so that we can reuse them. This is the only way or-patterns can be supported properly.

@folkertdev: What are your thoughts on this and on how and when you want to approach it?

In terms of the experiment... I expect we won't allow guaranteed-direct-jump using a non-fully-const value... The reason being that this requires inspecting the expression which is a weird sort of abstraction break (you wouldn't be able to do let x = Some(r); const continue 'blk x;).

The current implementation only supports integers and enums without fields as the scrutinee/state. Clearly we should never accept the abstraction break that you mention.

And without that, using patterns seems barely better than jump labels.

Whether or not we use patterns, what seems fairly elegant to me about this approach is that it keeps the jump labels in the value space which means that const computations can be performed to choose the jump target and that arbitrary other computations can be performed to choose the target when needed by leaving off the #[const_continue] (and accepting the codegen implications of that) without having to switch the entire block to a different syntax or engage in other workarounds.

Of course, I can imagine ways we could keep the jump labels in the value space without using patterns at all (rather than using them in restricted form by restricting the scrutinee type), and it'd be interesting to think through the pros and cons of that.

In terms of the experiment... I expect we won't allow guaranteed-direct-jump using a non-fully-const value... The reason being that this requires inspecting the expression which is a weird sort of abstraction break (you wouldn't be able to do let x = Some(r); const continue 'blk x;).

The current implementation only supports integers and enums without fields as the scrutinee/state. Clearly we should never accept the abstraction break that you mention.

The argument of the const continue is a const value. Either a literal const {} block or for convenience directly an integer or enum literal.

For one, I believe the static pattern-matching should use the const-eval interpreter instead of manually operating on valtrees. For two, it should not duplicate the work of match lowering; instead, BuiltMatchTree should track the tests cases that lead to each branch so that we can reuse them. This is the only way or-patterns can be supported properly.

Const eval needs a fully built MIR body, but we are currently building a MIR body, so there is nothing const eval can run on. As for BuiltMatchTree, that is already used and static_pattern_match handles or patterns already. Everything is just intentionally limited to integers, bools and fieldless enums to make the implementation easier.

Const eval needs a fully built MIR body

I meant we should manipulate the constant with a CompileTimeInterpCx instead of going through a valtree; we indeed cannot evaluate the MIR body we're building.

As for BuiltMatchTree, that is already used and static_pattern_match handles or patterns already.

Only top-level or-patterns are handled. The specific semantics of nested or-patterns (especially mixed with guards) is tricky and not handled by static_pattern_match at all. As I've said elsewhere, using BuiltMatchTree like this (i.e. by trying to match up each or-alternative with its branch in BuiltMatchTree by relying on the order in which they show up) is not robust. I am proposing to keep the TestCases around because I expect that would make your life much easier.

I do want to be clear that I'm ok with merging this PR as is (re MIR lowering), these are only remarks for future development of the feature.

Only top-level or-patterns are handled. The specific semantics of nested or-patterns (especially mixed with guards) is tricky and not handled by static_pattern_match at all.

Only integers, bools and fieldless enums are supported, none of which can have nested or-patterns. And guards are fundamentally incompatible with #[loop_match] as they require running arbitrary code at runtime to determine which match arm to take.

For one, I believe the static pattern-matching should use the const-eval interpreter instead of manually operating on valtrees. For two, it should not duplicate the work of match lowering; instead, BuiltMatchTree should track the tests cases that lead to each branch so that we can reuse them. This is the only way or-patterns can be supported properly.

folkertdev: What are your thoughts on this and on how and when you want to approach it?

For the types that we support right now (scalars and fieldless enums), the ValTree approach seems robust, so I'd prefer to stick with that right now. It can be improved later if we all agree that CompileTimeInterpCx is better or easier.

Whether or not we use patterns, what seems fairly elegant to me about this approach is that it keeps the jump labels in the value space which means that const computations can be performed to choose the jump target and that arbitrary other computations can be performed to choose the target when needed by leaving off the #[const_continue] (and accepting the codegen implications of that) without having to switch the entire block to a different syntax or engage in other workarounds.

To add to that: having the state as a value is extremely useful for state machines that are used in streaming where the state has to be stored. It's of course possible to keep track of updating the state on every state transition manually, but having the language do it for you is convenient.

I am proposing to keep the TestCases around because I expect that would make your life much easier.

@Nadrieril are you able to work on keeping the TestCases? The pattern match code is really quite daunting for an outsider.

We've processed the latest round of review comments

@rustbot ready

We'd like to get feedback and merge this now before we go too far in a direction that others have concerns with.

I continue to be skeptical of the loop-match phrasing. I don't think "no new syntax" is a good argument with "well except it adds 2 new attributes and only supports limited patterns and needs custom pattern lowering", because at that point it's essentially new syntax anyway. I'm still inclined to say that this should be working on block labels that can be jumped between, making those labelled blocks directly the states in the state machine.

But none of that is experiment-blocking. I'm fine with an experiment moving forward so we can see more real examples of what you want to do with this, and less "well that's just 'a: { continue 'b; }" examples.

@rustbot labels -I-lang-nominated +I-lang-radar

We discussed this in our lang call today. We're approving this experiment. I'll continue to be the lang "champion".

To emphasize what @scottmcm said above, which he also said in discussion, he'd like to see more motivating examples for why keeping the jump labels in the value space is important. As we go forward here and work to write design meeting documents and to revise the RFC, this is something that we'll want to keep in mind and be sure to address.

I continue to be skeptical of the loop-match phrasing. I don't think "no new syntax" is a good argument with "well except it adds 2 new attributes and only supports limited patterns and needs custom pattern lowering", because at that point it's essentially new syntax anyway.

Kinda, yeah. The crucial difference is that it doesn't need any changes to the parser and formatter, or any tooling like rust-analyzer. So the impact is much smaller.

more motivating examples for why keeping the jump labels in the value space is important.

Sure. In short, the main motivating example here is the zlib-rs decompression state machine. It is a streaming algorithm, meaning that it must be able to pause and save its state when it runs out of input, so it can resume again when more input arrives. Beyond the rust value language being a lot more expressive than the one for labels, the ability to easily store the state is the crucial reason to us for using enums, and not labels.

The code is at https://github.com/bjorn3/zlib-rs/blob/loop_match_attr/zlib-rs/src/inflate.rs#L871, just search for const_continue to see how it's used.

As we go forward here and work to write design meeting documents and to revise the RFC, this is something that we'll want to keep in mind and be sure to address.

Right. With this PR more or less complete, maybe we should strategize a bit what good concrete next steps are?

Only integers, bools and fieldless enums are supported

Does the feature not intend to go further than that? Again, I'm just saying "btw, here are my ideas for implementing the more complete version of this". Current state is ok for the experiment.

@Nadrieril are you able to work on keeping the TestCases?

Will do, I had plans along these lines already :) No promises on the delay tho.

Does the feature not intend to go further than that? Again, I'm just saying "btw, here are my ideas for implementing the more complete version of this". Current state is ok for the experiment.

We'll cover all types eventually, so we'll keep that in mind.

Will do, I had plans along these lines already :) No promises on the delay tho.

Sounds good! I'd be happy to do actual implementation work too, but in terms of design you'll have a much better idea of how the pieces do/should fit together.